-
Notifications
You must be signed in to change notification settings - Fork 1
Add a Pure attribute for functions and methods. #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think it's clear what the intention is, I do have some suggestions to make it more clear
* 4. Read from an object property on the same object unless that property is `readonly`. | ||
* 5. Call any function that is not also marked "pure." (It may invoke a callable passed to it as an explicit argument.) | ||
* | ||
* Many functions in the PHP standard library are already pure. Implementers MUST treat those as acceptable to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this more explicit somehow? Like a reference of functions? Right now it sounds like, you need to figure out yourself if functions are pure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list is... not small. 90% of the string and array functions would fall into this category. Listing them here would be a challenge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 Options IMO:
- Add a separate stub-document that contains all Pure functions
- Add the #[Pure] attribute to the docs
- Reference the PHPstorm stub file at https://github.com/JetBrains/phpstorm-stubs/
* In PHP, that means a pure function MUST NOT: | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to reread this part before I noticed that the Must not was a list of prohibited operations for pure functions.
Maybe we should repeat the "must not" on every line rather than just once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good. It also.allows one to mix requirements like
- MUST NOT do x
- SHALL do y
- but MUST do that
I'm missing what the added value is of marking a function as |
The use case would be for telling an SA tool "I expect this entire stack of function calls to be pure, and have no side effects. Make sure I'm correct." |
Couple of questions/remarks:
|
* | ||
* If this attribute is placed on a method in an interface, it means that all implementations of that method MUST | ||
* themselves be marked pure. Implementers MUST enforce this rule. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see a reference to where the information came.from so that people realize that this is not something we invented ourselves but is based on other peoples know-how.
In this case a link to the Wikipedia page seems to be a good idea.
Using either a1 or an {@ see} Annotation (oh the irony 😂)
Footnotes
-
footnote ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to add the requirement of references to the meta-document....
It looks like creating a template for a new attribute might be a good idea to provide some guidelines of what to think of and how to structure the information so that nothing important gets lost and the informations are roughly compareable. |
Indeed. The main point of this attribute is/was to give us a concrete example to chew on to develop said template. 😄 What sections do you see, and what format? Like, should we use Markdown in the docblock? |
Markdown is.most widely used, so that would probably be the most preferred option. I'd though prefere Restructured Text as it is much more feature-rich. If I recall correctly PHPDocumentor as well as GitHub can render ReST as well as Markdown, so this seems to be a technicallity. I'll try to start a template tomorrow in a separate PR |
|
||
/** | ||
* Indicates that a function or method is a "pure" function. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (also to consider for the template):
* | |
* | |
* Target audience: | |
* - Implementors: static analysis tools. | |
* - Users: packages which want to safeguard "pure" functions via static analysis tools. |
Phrasing may need work, but hopefully you get my drift.
phpDocumentor can render ReST, only for documents. Docblocks must be written in commonmark markdown. It has been like that for years. And most tooling is working like that. So if we want to add some structure it must be in markdown. |
Thanks for the input. Rewrote the ReST template in Markdown now 😁 |
This is the simplest attribute I can think of, so it should serve as a way for us to flesh out our process, format, etc. It's borrowed from the PHPStorm attributes (that I don't know anyone used).
The criteria of a pure function I copied directly from Wikipedia. The rest is my own writing. All subject to review, naturally.